-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Actionable Observability] - Add latency alert history chart on the Alert details page for APM #148011
[Actionable Observability] - Add latency alert history chart on the Alert details page for APM #148011
Conversation
…-ref HEAD~1..HEAD --fix'
x-pack/plugins/apm/public/hooks/use_fetch_triggered_alert_history.ts
Outdated
Show resolved
Hide resolved
@fkanout {
"index": ".alerts-observability.apm.alerts-*",
"size": 0,
"query": {
"bool": {
"must": [
{
"term": {
"kibana.alert.rule.uuid": "aa081910-969e-11ed-adcb-b1469b179bd9"
}
},
{
"range": {
"kibana.alert.time_range": { "gte": "now-30d", "lt": "now" }
}
}
]
}
},
"aggs": {
"histogramTriggeredAlerts": {
"date_histogram": {
"field": "kibana.alert.start",
"fixed_interval": "1d",
"extended_bounds": { "min": "now-30d", "max": "now" }
}
},
"avgTimeToRecoverMSE": {
"filter": {
"term": {
"kibana.alert.status": "recovered"
}
},
"aggs": {
"recoveryTime": {
"avg": {
"field": "kibana.alert.duration.us"
}
}
}
}
}
} And for this, you need to change the
Basically, we cannot use and allow |
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM -> Type and query looks good!!!
@kdelemme thank you for pairing with me yesterday to fix the type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me... tested in browser.
...components/alerting/ui_components/alert_details_app_section/latency_alerts_history_chart.tsx
Outdated
Show resolved
Hide resolved
errorTriggeredAlertHistory?: string; | ||
triggeredAlertsData?: FetchTriggeredAlertsHistory; | ||
} | ||
export function useFetchTriggeredAlertsHistory({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels like most of this belongs as a client in the alerts plugin. What's specific to APM here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is a client, we can import it in apm and use it like:
const alertsHistory = await alertsClient.getTriggerdAlertsHistory({ features: 'apm', ruleId })
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good idea, can we create an issue and have follow up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -287,11 +279,22 @@ export const metricsAggsSchemas = t.exact( | |||
}), | |||
}) | |||
), | |||
aggs: t.undefined, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what Xavier did here? We added these two attributes to not allow user to create nested aggs on metrics aggregations knowing that ES won't like it. It was to make our API nicer/smarter. However, this created a nightmare to combine metrics and bucket aggs together. So We decide to remove these two attributes and let ES tell you when the aggs is not formatted correctly. So now our validation of type is simpler and cleaner to use.
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @fkanout |
Summary
Closes #147932 by adding a new latency chart that covers the last 30 days of alerts for a given rule.
And it adds annotations with the number of alerts for a given day besides the time to recover.